Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix SQLite current DB detection #5524

Closed
wants to merge 1 commit into from
Closed

Fix SQLite current DB detection #5524

wants to merge 1 commit into from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Jul 20, 2022 •
edited
Loading

Q A
Type bug
Fixed issues #5046

Summary

SQLite support schemas concept. There is a main schema named main and other schemas can be attached with the ATTACH SQL statement.

Citing https://www.sqlite.org/pragma.html:

A pragma may have an optional schema-name before the pragma name. The schema-name is the name of an ATTACH-ed database or "main" or "temp" for the main and the TEMP databases. If the optional schema name is omitted, "main" is assumed. In some pragmas, the schema name is meaningless and is simply ignored. In the documentation below, pragmas for which the schema name is meaningful are shown with a "schema." prefix.

This PR fixes the current DB detection which is always main. All tests pass. Later I plan to provide a PR to adjust the SQLitePlatform to support working with the attached/different databases.

@mvorisek mvorisek marked this pull request as ready for review July 20, 2022 11:11
@morozov
Copy link
Member

morozov commented Jul 20, 2022 •
edited
Loading

In its current state, this is a change for the sake of a change.

Later I plan to provide a PR to adjust the SQLitePlatform to support working with the attached/different databases.

It might be acceptable as part of the above improvement but I don't see a point in making this change now.

@mvorisek
Copy link
Contributor Author

Currently I fix the SQLitePlatform::getCurrentDatabaseExpression() return value, as empty string is unusable - https://dbfiddle.uk/?rdbms=sqlite_3.27&fiddle=1a3e061af85121cf3d3df64b5234cb5d - and main should be returned.

@morozov
Copy link
Member

morozov commented Jul 21, 2022

Closing as this change doesn't address any issue. It could be implemented as part of the support for multiple schemas or databases for the SQLite platform.

@morozov morozov closed this Jul 21, 2022
@mvorisek
Copy link
Contributor Author

Well, the SQLite docs and dbfiddle.uk demo clearly shows that the current database is always main. Why do I need to provide more changes in this PR when this change can and should be integrated separately, so it can be used in projects right immediatelly and if there is any issue, it can be debug with smaller commit.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Databases SQLite Won't Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mvorisek @morozov